Skip to content

Feature/clean thumbnails avatars#85

Open
zerox80 wants to merge 6 commits intoopencloud-eu:mainfrom
zerox80:feature/clean-thumbnails-avatars
Open

Feature/clean thumbnails avatars#85
zerox80 wants to merge 6 commits intoopencloud-eu:mainfrom
zerox80:feature/clean-thumbnails-avatars

Conversation

@zerox80
Copy link
Contributor

@zerox80 zerox80 commented Jan 19, 2026

Thumbnail Cache Refactoring and Avatar Fixes

Summary

This pull request refactors the thumbnail caching mechanism to improve performance and stability, and includes critical fixes for user avatar loading. The legacy ThumbnailsCacheManager has been replaced by an optimized ThumbnailsRequester implementation, and several thread safety issues have been resolved.

Technical Details

Thumbnail Caching

  • Refactoring: Completely removed ThumbnailsCacheManager and DiskLruImageCache in favor of a modern, streamlined approach within ThumbnailsRequester.
  • Thread Safety: Addressed race conditions in ThumbnailsRequester when initializing the shared disk and memory caches. Access to these caches is now properly synchronized.
  • Optimization: Adjusted thumbnail sizing logic to prevent memory waste and improve loading times.
  • Dead Code Removal: Cleaned up unused legacy code and resources related to the old caching system.

Avatar Loading

  • Graph API Integration: Updated the user avatar loading logic to utilize the Graph API endpoint, ensuring reliable retrieval of user profile images.
  • Cache Control: Fixed issues with avatar cache invalidation and loading control to ensure users always see the correct profile picture.
  • Bug Fixes: Resolved specific crashes and loading failures related to avatar requests in various parts of the UI (Drawer, Toolbar, Upload List).

Other Changes

  • Code Quality: Resolved extensive Detekt issues, including formatting and spacing violations.
  • Stability: Fixed potential crashes caused by incorrect context usage or missing null checks in the thumbnail generation flow.

Impact

These changes significantly reduce the maintenance burden by removing complex legacy caching code and provide a smoother user experience with faster and more reliable thumbnail and avatar loading.

Testing

  • Verified that file thumbnails load correctly in the file list and grid view.
  • Verified that user avatars appear correctly in the navigation drawer and toolbar.
  • Confirmed that removing the legacy cache classes does not introduce regressions in image loading.

@zerox80 zerox80 force-pushed the feature/clean-thumbnails-avatars branch 2 times, most recently from 6f322b4 to 72d55fa Compare January 20, 2026 08:05

private const val SPACE_SPECIAL_PREVIEW_URI = "%s?scalingup=0&a=1&x=%d&y=%d&c=%s&preview=1"
private const val FILE_PREVIEW_URI = "%s%s?x=%d&y=%d&c=%s&preview=1&id=%s"
private const val FILE_PREVIEW_URI = "%s/remote.php/webdav%s?x=%d&y=%d&c=%s&preview=1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this looks good, OpenCloud is not PHP anymore.

@guruz
Copy link
Contributor

guruz commented Feb 10, 2026

Thumbnail Caching

I haven't really looked at the code yet. Where is stuff cached? From the names it seems it was Disk(Lru) before, what is it now?

@guruz
Copy link
Contributor

guruz commented Feb 24, 2026

FYI I'm looking / testing this PR now. @zerox80

@zerox80
Copy link
Contributor Author

zerox80 commented Feb 25, 2026

Hi, ill take a look now, was busy sorry

@zerox80
Copy link
Contributor Author

zerox80 commented Feb 25, 2026

re-test please @guruz

@zerox80
Copy link
Contributor Author

zerox80 commented Feb 25, 2026

Thumbnail Caching

I haven't really looked at the code yet. Where is stuff cached? From the names it seems it was Disk(Lru) before, what is it now?

We completely moved away from the old manual DiskLruCache handling and now rely on Coil's built-in caching mechanism, which is much more efficient and modern.

It uses a two-tier caching strategy (both Memory and Disk cache), which is configured centrally in the ThumbnailsRequester.kt

Disk Cache: We use Coil's DiskCache.Builder() to store thumbnails in the app's cache directory (thumbnails_coil_cache). It is explicitly limited to 100MB to ensure it doesn't inflate the app storage endlessly. (Location: appContext.cacheDir.resolve("thumbnails_coil_cache"))

Memory Cache: We use Coil's MemoryCache.Builder() configured to use up to 25% of the available application memory (maxSizePercent(0.25)). This drastically improves scrolling performance in lists/grids because decoded images are kept directly in memory.

Both caches are shared as Singletons (sharedDiskCache and sharedMemoryCache) and passed to the ImageLoader instance, so the caching rules apply globally to all thumbnail requests handled by Coil

This commit acts as a squash of the thumbnail cache refactoring and avatar fixes.

See PR description for full details.
- Update FILE_PREVIEW_URI to use direct native /webdav path.
- Use Graph API explicitly for fetching Avatar image, dropping the /index.php/avatar hack & interceptor fallbacks.
@zerox80 zerox80 force-pushed the feature/clean-thumbnails-avatars branch from a5268f3 to a5e37bc Compare February 25, 2026 15:06
@guruz
Copy link
Contributor

guruz commented Feb 26, 2026

(Don't do any changes now on top as I'm working on your branch myself now)

I actually think your code is regressing the cache by not going to the cache first if we had a preview already in (disk) cache.
So it makes the behavior of the app worse by not showing images when being offline or unauthorized.

I'll check if there is a goo way to fix this..

@guruz
Copy link
Contributor

guruz commented Feb 26, 2026

Or maybe I take this back.
It seems this happened when the server sent 401 because token expired
When there was a network error (UnknownHost or whatever) it actually went succesfully to the cache

02-26 12:31:12.678 14205 14205 I RealImageLoader: 💾 Successful (DISK) - https://demo.opencloud.eu/webdav/OpenCloud-UI-Tab_Work_Web_2400px.jpg?x=1024&y=1024&c=&preview=1

Can ignore this for now then :)

@guruz
Copy link
Contributor

guruz commented Feb 26, 2026

Can ignore this for now then :)

Or can not ignore this. Because 401 is actually quite likely if access_token has low expiry time.

But maybe the actual fix is not related to here, but re-ordering how ConnectionValidator & friends behave or how token expiry is detected.

Just notes to myself..

@zerox80
Copy link
Contributor Author

zerox80 commented Feb 26, 2026

Hey @guruz, just to share some thoughts on why the cache behaves differently between UnknownHostException and 401 Unauthorized:

The difference lies in how OkHttp/Coil handles HTTP responses versus network exceptions when checking the disk cache. When Coil tries to load an image, it might need to validate a stale cache entry.

If the device is offline, OkHttp throws an IOException (like UnknownHostException). Coil/OkHttp Cache catches this and correctly falls back to returning the stale disk cache entry.
However, if the token is expired, the server responds with an HTTP 401. To OkHttp/Coil, this is a successful network transaction (no IOException is thrown). Since the server explicitly rejected the request, OkHttp doesn't fall back to the stale cache; it just returns the 401 response to Coil, which then fails the image load.
Possible solutions to consider:

Refresh on 401: Add an OkHttp Authenticator (or Interceptor) to the client used by Coil. When it catches a 401, it can synchronously refresh the token via AccountManager and retry the image request.
Mask 401 as Offline: If token refresh fails or isn't possible right away, an OkHttp Interceptor could catch the 401 and explicitly throw an IOException("Unauthorized - Token Expired"). By throwing an exception instead of returning the 401 response, it would trick Coil into treating it exactly like an offline scenario, and it will serve the stale image from the disk cache.

@zerox80
Copy link
Contributor Author

zerox80 commented Feb 26, 2026

Hey, so there was a bug that TUS Upload doesn't get an Etag? Somehow?

its fixed now, please test

@guruz
Copy link
Contributor

guruz commented Feb 26, 2026

@zerox80 Thanks for spotting/fixing! I'll try it out..

@TheOneRing CC
Seems the desktop client also has this flow where it can receive an ETag or not and then do PROPFIND. https://github.com/opencloud-eu/desktop/blob/c7c05eaadf5d9cb53c95f9c6872703a9d70ba3fb/src/libsync/propagateuploadtus.cpp#L260
Wondering why we don't always get one, is that storage specific?
old upstream discussion see here: owncloud/ocis#216 (comment)

@zerox80 zerox80 force-pushed the feature/clean-thumbnails-avatars branch from 4c430f5 to 68322d8 Compare February 26, 2026 17:49
TUS PATCH responses from openCloud do not include ETag headers.
This caused large file uploads (>10MB) to have null ETags in the
local database, breaking thumbnail preview URLs and caching.

Changes:
- TusUploadHelper: after TUS upload completes, perform a PROPFIND
  to fetch the file's ETag from the server
- PatchTusUploadChunkRemoteOperation: extract ETag from PATCH
  response header (for future backend support)
- UploadFileFromFileSystemWorker: persist ETag to database for both
  overwrite and non-overwrite uploads
- ThumbnailsRequester: use .orEmpty() for null-safe ETag formatting
@zerox80 zerox80 force-pushed the feature/clean-thumbnails-avatars branch from 68322d8 to 4bedd0c Compare February 26, 2026 17:56
@zerox80
Copy link
Contributor Author

zerox80 commented Feb 26, 2026

@zerox80 Thanks for spotting/fixing! I'll try it out..

@TheOneRing CC Seems the desktop client also has this flow where it can receive an ETag or not and then do PROPFIND. https://github.com/opencloud-eu/desktop/blob/c7c05eaadf5d9cb53c95f9c6872703a9d70ba3fb/src/libsync/propagateuploadtus.cpp#L260 Wondering why we don't always get one, is that storage specific? old upstream discussion see here: owncloud/ocis#216 (comment)

The Tusd library has a hook called PreFinishResponseCallback, with that REVA could calculate the etag after the last chunk and write it to the response header, but this hook is never used....

Is it atleast planned? Should we open Issue on Opencloud backend repo?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants